fix: restore wrap anchor with paint retries#134
Conversation
Greptile SummaryThis PR replaces the hard-coded Key changes:
Confidence Score: 5/5Safe to merge — the change is a clean drop-in replacement with no correctness issues. No P0 or P1 issues found. The new No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["useLayoutEffect fires\n(wrapLines changed)"] --> B{anchor found?}
B -- no --> C[update prev refs]
B -- yes --> D["restoreViewportAnchor()\n(immediate, synchronous)"]
D --> E["suppressNextSelectionAutoScroll = true"]
E --> F["schedulePaintRetries(restoreViewportAnchor, 2)"]
F --> G["scheduleNext(2)\n→ push cancel1"]
G --> H["Frame 1 (rAF / setTimeout 16ms)\ncallback() + scheduleNext(1)\n→ push cancel2"]
H --> I["Frame 2 (rAF / setTimeout 16ms)\ncallback()"]
F --> J["return cancelFn\n(cancels.splice(0).forEach)"]
J --> K["Effect cleanup\ncancels all pending frames"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function schedulePaintRetries(callback: () => void, frames = 2) { | ||
| const cancels: Array<() => void> = []; | ||
| const requestFrame = | ||
| typeof globalThis.requestAnimationFrame === "function" | ||
| ? (next: () => void) => { | ||
| const frameId = globalThis.requestAnimationFrame(() => next()); | ||
| return () => { | ||
| if (typeof globalThis.cancelAnimationFrame === "function") { | ||
| globalThis.cancelAnimationFrame(frameId); | ||
| } | ||
| }; | ||
| } | ||
| : (next: () => void) => { | ||
| const timeout = setTimeout(next, 16); | ||
| return () => clearTimeout(timeout); | ||
| }; | ||
|
|
||
| const scheduleNext = (remaining: number) => { | ||
| const cancel = requestFrame(() => { | ||
| callback(); | ||
| if (remaining > 1) { | ||
| scheduleNext(remaining - 1); | ||
| } | ||
| }); | ||
| cancels.push(cancel); | ||
| }; | ||
|
|
||
| scheduleNext(frames); | ||
| return () => { | ||
| cancels.splice(0).forEach((cancel) => cancel()); | ||
| }; |
There was a problem hiding this comment.
cancels array
After a frame fires, its cancel handle stays in the cancels array even though calling cancelAnimationFrame on a frame that has already executed is a harmless no-op. With frames=2:
scheduleNext(2)→cancels = [cancel1]- Frame 1 fires →
scheduleNext(1)→cancels = [cancel1, cancel2](cancel1 is now stale) - Frame 2 fires →
cancels = [cancel1, cancel2](both stale if cleanup runs here)
The cleanup calls both, but cancel1 and cancel2 become no-ops once their respective frames have fired. This is correct in all paths but results in unnecessary no-op calls. A minor defensive improvement would be to splice the cancel out of the array once its frame has run:
const scheduleNext = (remaining: number) => {
const cancel = requestFrame(() => {
const idx = cancels.indexOf(cancel);
if (idx !== -1) cancels.splice(idx, 1);
callback();
if (remaining > 1) {
scheduleNext(remaining - 1);
}
});
cancels.push(cancel);
};This is low priority since cancelAnimationFrame/clearTimeout on already-fired IDs is well-defined as a no-op in all major environments.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Closing for backlog cleanup. This wrap-anchor fix has since been overtaken by later wrap/viewport work, so it does not seem worth keeping open in its current form. |
1 participant
Summary
requestAnimationFrameand fall back to timed frames in testsTesting
bun run typecheckbun testbun run test:tty-smoketimeout 5 script -q -f -e -c "bun run src/main.tsx -- diff <before> <after>" <transcript>This PR description was generated by Pi using OpenAI GPT-5